Add upload to registry workflow for foundry_std #2461

THenry14 · 2024-09-11T10:00:29Z

This PR is a part of the stack:
-- Add missing info recommended for package uploads (#2460)
--> Add github action workflow for automatic registry uploads during release (This PR)

Introduced changes

Updates github workflow to also upload std libraries to scarbs registry during release
Sets snforge_scarb_plugin to specific version downloadable from the registry, as required by scarb

Checklist

franciszekjob · 2024-10-11T10:12:44Z

Blocked by software-mansion/scarb#1605

Related to #2344 This PR is a part of the stack: --> Add missing info recommended for package uploads (This PR) -- Add github action workflow for automatic registry uploads during release (#2461) ## Introduced changes  - Adds readmes and links to respective Scarb.tomls, so to view this metadata on the registry and supress scarb warnings - Bump snforge_scarb_plugin version to `0.31.0`, to properly reflect its version in regards to the registry. **From now on all changes made to this crate should be concluded with version bump** ## Checklist  - [X] Linked relevant issue - [X] Updated relevant documentation - [X] Added relevant tests - [X] Performed self-review of the code - [X] Added changes to `CHANGELOG.md` --------- Co-authored-by: Fiiranek <[email protected]> Co-authored-by: Franciszek Job <[email protected]>

This reverts commit da4a5b8.

kkawula · 2024-11-04T10:13:41Z

It needs clarification, but plugins weren't correctly handled by scarb until software-mansion/scarb#1712 was merged, I suppose that it will be needed to update scarb version in the entire project.
In the earlier versions scarb had problem with access to the cache in database which caused dead lock.
CI to lookup: https://github.com/foundry-rs/starknet-foundry/actions/runs/11577774705

edit: The scarb version in the project can probably stay as it is but while package upload dependency in snforge_std/Scarb.toml should be manually switched to registry dependency and after upload reverted.

maciektr · 2024-11-04T15:43:12Z

It needs clarification, but plugins weren't correctly handled by scarb until software-mansion/scarb#1712 was merged, I suppose that it will be needed to update scarb version in the entire project.

It shouldn't have anything to do with uploading packages though

kkawula · 2024-11-13T11:05:34Z

.github/workflows/publish_packages.yml

+        working-directory: snforge_std
+        run: |
+          ../scripts/set_plugin_version.sh
+          scarb publish --allow-dirty


--allow-dirty because we modify snforge_std by executing the script without committing changes

Why not require from tag that triggers the workflow to already include changes introduced by set_plugin_version.sh? 🤔

For development reason, we always would like to have a relative path specified in the dependency

Is it a hard requirement to set the version in snforge only for uploads? What are the downsides of having this set in repo?

.github/workflows/publish_packages.yml

kkawula · 2024-11-13T11:10:25Z

it works, tested on dev registry.

DelevoXDG

Partial review, please re-request when ready 👍🏻

DelevoXDG · 2024-11-13T11:45:51Z

.github/workflows/publish_packages.yml

+          # todo: Use scarb stable version that support publishing plugins (after 2.8.4)
+          scarb-version: "nightly-2024-11-09"
+
+      - name: Check if package version exist in the registry


Is this step necessary?

We implemented this for publishing Scarb plugins because their version matches the Cairo version. Since multiple Scarb versions can correspond to a single Cairo version, running the workflow with every Scarb release could cause errors by publishing plugins with the same version.

Afaik, this doesn’t apply to snfoundry plugins because their version isn’t tied to any Cairo version. Normally, the workflow should only be triggered once per tag.

We might decouple foundry plugin version from foundry itself so it might be the case that for consecutive foundry releases, plugin version will stay the same.

So we have to make sure we don't reupload the plugin to the registry in these cases.

Sometimes we patch plugin, without release then we would like to publish it and sometimes we will not do release plugin together with snforge_std and sncast_std, we want to set it on 1.0.0 and keep it until there are changes, and then it would fail the workflow and CI will be red at the release

As I pointed out offline, if you're going in the direction of decoupling plugin from foundry itself, it seems to me it a separate workflow should be considered

It is one of the opportunities, but isn't it good enough? @cptartur

As on how the decoupling will go exactly is yet to be determined. I think the workflow in the current form is okay, and we can introduce changes in the future.

Similarly to @DelevoXDG , I would probably be in favour of splitting release flows for macros and standard libraries as well.

My understanding is that the plugin won't necessarily follow the *_std version, so it may make sense to release it independently. For this manual workflow dispatch only is good enough. Most of logic from this file is fine, though you wouldn't need anything from this step (I mean the check-versions, which I find very hacky) so I'd actually argue it could become far simpler.

Having this, we could probably incorporate *_std upload to the release.yml workflow, as it seems to me this is more natural place for this. We already verify the version there, so chances are we do not need any further modifications - we could probably just add a new job (standard-libraries-release, or scarbs-release or sth) that will set the plugin version (this is unnecessary if you decide to set the version in Scarb.toml in repo) and use scarb to publish and call it a day. No need to check versions against the registry, verify what should/should not be published etc.

Thoughts?

.github/workflows/publish_packages.yml

cptartur · 2024-11-13T12:08:05Z

.github/workflows/publish_packages.yml

+on:
+  release:
+    types:
+      - published


I think published will also trigger on creating a draft release which our workflow does. I think it should be released so it only uploads to registry if we publish a new release.

Also a concern, in case we ever create a pre-release, do we want to upload to registry as well? Does it support pre-release versions?

cc @maciektr

You are right, it should be changed to released
https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=released#release

Afaik published is triggered when you publish a draft - regardless of whether it's pre-release or release.

released differentiates between pre-release and full release.

in case we ever create a pre-release, do we want to upload to registry as well

Why not? 🤔

Okay, if we want to upload pre-releases as well, let's keep it at published.

.github/workflows/publish_packages.yml

cptartur · 2024-11-13T12:12:51Z

.github/workflows/publish_packages.yml

+          # todo: Use scarb stable version that support publishing plugins (after 2.8.4)
+          scarb-version: "nightly-2024-11-09"
+
+      - name: Check if package version exist in the registry


We might decouple foundry plugin version from foundry itself so it might be the case that for consecutive foundry releases, plugin version will stay the same.

So we have to make sure we don't reupload the plugin to the registry in these cases.

.github/workflows/publish_packages.yml

scripts/set_plugin_version.sh

This reverts commit 9ce17e1.

cptartur

@THenry14 Can you please review as well? I know it's technically your PR but it's changed a bit 😅

cptartur · 2024-11-14T11:14:37Z

.github/workflows/publish_packages.yml

+on:
+  release:
+    types:
+      - released


Following the discussion from different comment

Suggested change

- released

- published

.github/workflows/publish_packages.yml

THenry14 · 2024-11-14T16:46:04Z

.github/workflows/publish_packages.yml

+          # todo: Use scarb stable version that support publishing plugins (after 2.8.4)
+          scarb-version: "nightly-2024-11-09"
+
+      - name: Check if package version exist in the registry


Similarly to @DelevoXDG , I would probably be in favour of splitting release flows for macros and standard libraries as well.

My understanding is that the plugin won't necessarily follow the *_std version, so it may make sense to release it independently. For this manual workflow dispatch only is good enough. Most of logic from this file is fine, though you wouldn't need anything from this step (I mean the check-versions, which I find very hacky) so I'd actually argue it could become far simpler.

Having this, we could probably incorporate *_std upload to the release.yml workflow, as it seems to me this is more natural place for this. We already verify the version there, so chances are we do not need any further modifications - we could probably just add a new job (standard-libraries-release, or scarbs-release or sth) that will set the plugin version (this is unnecessary if you decide to set the version in Scarb.toml in repo) and use scarb to publish and call it a day. No need to check versions against the registry, verify what should/should not be published etc.

Thoughts?

THenry14 · 2024-11-14T16:52:05Z

scripts/set_plugin_version.sh

+
+PLUGIN_FILE_PATH="../crates/snforge-scarb-plugin/Scarb.toml"
+
+VERSION=$(cat "$PLUGIN_FILE_PATH" | grep version | cut -d '"' -f 2)


useless use of cat

https://www.shellcheck.net/wiki/SC2002

This probably explains it better
https://en.wikipedia.org/wiki/Cat_(Unix)#Useless_use_of_cat

THenry14 · 2024-11-14T16:59:12Z

.github/workflows/publish_packages.yml

+        working-directory: snforge_std
+        run: |
+          ../scripts/set_plugin_version.sh
+          scarb publish --allow-dirty


Is it a hard requirement to set the version in snforge only for uploads? What are the downsides of having this set in repo?

Add required info for scarb packages

1e9a5ca

This was referenced Sep 11, 2024

Add info for scarb packages uploads #2460

Merged

Remove scarb version constraint for foundry_std registry uploads #2462

Open

Add upload to registry workflow for foundry_std

6e31415

THenry14 force-pushed the szymczyk/2344-automatic-registry-uploads branch from 177666e to 6e31415 Compare September 11, 2024 10:18

maciektr mentioned this pull request Sep 17, 2024

Support packaging cairo-plugins in Scarb software-mansion/scarb#1527

Closed

maciektr mentioned this pull request Oct 8, 2024

Add CI workflow to publish starknet-foundry packages to registry software-mansion/scarb#1638

Closed

franciszekjob added the blocked label Oct 11, 2024

Base automatically changed from szymczyk/registry to master October 11, 2024 16:12

franciszekjob removed the blocked label Oct 15, 2024

kkawula added 6 commits October 29, 2024 14:24

Merge branch 'master' into szymczyk/2344-automatic-registry-uploads

048c7a1

Update snforge_scarb_plugin version 0.2.0 -> 0.32.0

017c24a

Update release script

c38c01c

Add empty line

fe53bbc

Update .tool-versions scarb 2.7.0 -> scarb 2.8.4

da4a5b8

Revert "Update .tool-versions scarb 2.7.0 -> scarb 2.8.4"

e438770

This reverts commit da4a5b8.

kkawula self-assigned this Nov 4, 2024

kkawula added 2 commits November 4, 2024 14:07

Update setup-scarb workflow version

0c27383

Add todo

ccbb537

kkawula added 7 commits November 5, 2024 16:20

Merge branch 'master' into szymczyk/2344-automatic-registry-uploads

61e7d8e

Restore old path

b6d75ef

Add version changing scripts

cac4ad5

Add running only on tag creation

c8266e6

Upadte TODO

6dd5e4b

Remove unnecessary code

c9a02d7

Update workflow

d7dbf6b

kkawula added 6 commits November 13, 2024 00:04

Add ispublished validation

b2f91a1

Update script

d943e43

Create new workflow file

e8dfaa1

Add empty line

e304776

Working state

c578bd3

Merge branch 'master' into szymczyk/2344-automatic-registry-uploads

979faab

kkawula reviewed Nov 13, 2024

View reviewed changes

.github/workflows/publish_packages.yml Show resolved Hide resolved

kkawula marked this pull request as ready for review November 13, 2024 11:10

kkawula requested review from cptartur, ddoktorski and franciszekjob as code owners November 13, 2024 11:10

kkawula marked this pull request as draft November 13, 2024 11:42

DelevoXDG reviewed Nov 13, 2024

View reviewed changes

Update version checking

3c35ec6

cptartur requested changes Nov 13, 2024

View reviewed changes

kkawula added 5 commits November 13, 2024 13:57

Improve readability

bbcd1a9

Update script

f31d8b8

Update script

9ce17e1

Revert "Update script"

9badfea

This reverts commit 9ce17e1.

Change workflow trigger

3eebe90

kkawula marked this pull request as ready for review November 13, 2024 13:54

kkawula requested a review from cptartur November 14, 2024 10:51

cptartur reviewed Nov 14, 2024

View reviewed changes

THenry14 commented Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add upload to registry workflow for foundry_std #2461

Add upload to registry workflow for foundry_std #2461

THenry14 commented Sep 11, 2024 •

edited

Loading

franciszekjob commented Oct 11, 2024

kkawula commented Nov 4, 2024 •

edited

Loading

maciektr commented Nov 4, 2024

kkawula Nov 13, 2024

DelevoXDG Nov 13, 2024

kkawula Nov 13, 2024

THenry14 Nov 14, 2024

kkawula commented Nov 13, 2024

DelevoXDG left a comment

DelevoXDG Nov 13, 2024

cptartur Nov 13, 2024

kkawula Nov 13, 2024 •

edited

Loading

DelevoXDG Nov 13, 2024

kkawula Nov 14, 2024

cptartur Nov 14, 2024

THenry14 Nov 14, 2024

cptartur Nov 13, 2024

cptartur Nov 13, 2024

kkawula Nov 13, 2024

maciektr Nov 13, 2024 •

edited

Loading

cptartur Nov 14, 2024

cptartur Nov 13, 2024

cptartur left a comment

cptartur Nov 14, 2024

THenry14 Nov 14, 2024

THenry14 Nov 14, 2024

THenry14 Nov 14, 2024

THenry14 Nov 14, 2024


		PLUGIN_FILE_PATH="../crates/snforge-scarb-plugin/Scarb.toml"

		VERSION=$(cat "$PLUGIN_FILE_PATH" \| grep version \| cut -d '"' -f 2)

Add upload to registry workflow for foundry_std #2461

Are you sure you want to change the base?

Add upload to registry workflow for foundry_std #2461

Conversation

THenry14 commented Sep 11, 2024 • edited Loading

Introduced changes

Checklist

franciszekjob commented Oct 11, 2024

kkawula commented Nov 4, 2024 • edited Loading

maciektr commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkawula commented Nov 13, 2024

DelevoXDG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkawula Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciektr Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cptartur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

THenry14 commented Sep 11, 2024 •

edited

Loading

kkawula commented Nov 4, 2024 •

edited

Loading

kkawula Nov 13, 2024 •

edited

Loading

maciektr Nov 13, 2024 •

edited

Loading